Skip to content

Do not block on duplicate base classes#15367

Merged
sobolevn merged 4 commits intomasterfrom
issue-15349
Jun 5, 2023
Merged

Do not block on duplicate base classes#15367
sobolevn merged 4 commits intomasterfrom
issue-15349

Conversation

@sobolevn
Copy link
Copy Markdown
Member

@sobolevn sobolevn commented Jun 4, 2023 •
edited
Loading

@JelleZijlstra I don't really have a prefence on this issue. I am fine with both blocking and non-blocking errors here.

But, it looks like that making this non-blocking is quite straight-forward.
Closes #15349

@github-actions

This comment has been minimized.

Sign in to view
JelleZijlstra
JelleZijlstra reviewed Jun 4, 2023
View reviewed changes
Comment thread test-data/unit/check-classes.test Outdated
...

b: B
# We use dummy type info, so no real attrs will be there:
Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra Jun 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pretty unfortunate, can we make it inherit from Any or just skip the duplicate base instead?

Copy link
Copy Markdown
Member Author

@sobolevn sobolevn Jun 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeap, got it. Fixed!

@github-actions

This comment has been minimized.

Sign in to view
@sobolevn sobolevn requested a review from JelleZijlstra June 4, 2023 17:43
hauntsaninja
hauntsaninja reviewed Jun 4, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

One possible improvement here could be to deduplicate the base classes when calculating MRO, so instead of Any MRO we still get the MRO as if the base class was unduplicated.

Comment thread test-data/unit/check-classes.test Outdated
@sobolevn
Copy link
Copy Markdown
Member Author

sobolevn commented Jun 4, 2023 •
edited
Loading

@hauntsaninja I don't fill comfortable doing this. Because we can have two generic bases: Some[int] and Some[str] - which one we should remove? This way we can generate strange errors that won't help much. Any is the default way to handle errors inside, so let's stick to it.

@hauntsaninja
Copy link
Copy Markdown
Collaborator

hauntsaninja commented Jun 4, 2023

Okay! I went ahead and added a test case for that situation

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 4, 2023

Diff from mypy_primer, showing the effect of this PR on open source code:

vision (https://github.com/pytorch/vision) got 1.36x slower (40.8s -> 55.5s)

@sobolevn sobolevn merged commit 66bd2c4 into master Jun 5, 2023
@sobolevn sobolevn deleted the issue-15349 branch June 5, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hauntsaninja hauntsaninja hauntsaninja left review comments

@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

“Duplicate base class” blocks checking

3 participants

@sobolevn @hauntsaninja @JelleZijlstra
⚡